Conversation
|
@awdavidson This looks like a nice addition. Could you add a test as well? |
pyiceberg/catalog/hive.py
Outdated
| @staticmethod | ||
| def _create_hive_client(properties: Dict[str, str]) -> _HiveClient: | ||
| uris = properties["uri"].split(",") | ||
| for idx, uri in enumerate(uris): | ||
| try: | ||
| return _HiveClient(uri, properties.get("ugi")) | ||
| except BaseException as e: | ||
| if idx + 1 == len(uris): | ||
| raise e | ||
| else: | ||
| continue |
There was a problem hiding this comment.
Can I make a suggestion to make it a bit more Pythonic (and easier to follow IMHO):
| @staticmethod | |
| def _create_hive_client(properties: Dict[str, str]) -> _HiveClient: | |
| uris = properties["uri"].split(",") | |
| for idx, uri in enumerate(uris): | |
| try: | |
| return _HiveClient(uri, properties.get("ugi")) | |
| except BaseException as e: | |
| if idx + 1 == len(uris): | |
| raise e | |
| else: | |
| continue | |
| @staticmethod | |
| def _create_hive_client(properties: Dict[str, str]) -> _HiveClient: | |
| uris = properties["uri"] | |
| e = ValueError(f"Invalid URI: {uris}") | |
| for uri in uris.split(","): | |
| try: | |
| return _HiveClient(uri, properties.get("ugi")) | |
| except BaseException as e: | |
| pass | |
| raise e |
There was a problem hiding this comment.
Rather than throwing a generic ValueError I have updated to capture and throw the last exception if that code path is reached. Let me know what you think
Unit tests have been added |
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
Thanks for adding this!
Regarding tests, I think we should cover the following scenario
- 1 uri
- 2 uris; both passes
- 2 uris; 1 fail, fallback to the second
- 2 uris; both fail, error
- (optional) 2 uris; 1 pass, second one not tried
| } | ||
|
|
||
| with patch('pyiceberg.catalog.hive._HiveClient') as mock_hive_client: | ||
| mock_hive_client.side_effect = [Exception("Connection failed"), MagicMock()] |
There was a problem hiding this comment.
does this mean the first connection will fail?
There was a problem hiding this comment.
Thank you for your comments. Yes this means the first connections will fail.
Scenario 2 uris; both passes will never be a valid case, if the first uri passes the second is never tried. So the only additional test would be to add one covering (optional) 2 uris; 1 pass, second one not tried
There was a problem hiding this comment.
thanks! i think that should cover all the cases
tests/catalog/test_hive.py
Outdated
| mock_hive_client.assert_any_call("thrift://localhost:10000", "user") | ||
| mock_hive_client.assert_any_call("thrift://localhost:10001", "user") |
There was a problem hiding this comment.
nit: assert that both 10000 and 10001 are called once, assert_called_once_with
kevinjqliu
left a comment
kevinjqliu
left a comment
There was a problem hiding this comment.
LGTM! Thank you for covering the test scenarios
Co-authored-by: Kevin Liu <kevinjqliu@users.noreply.github.com>
pyiceberg/catalog/hive.py
Outdated
| if last_exception is not None: | ||
| raise e | ||
| else: | ||
| raise ValueError(f"Unable to connect to hive using uri: {properties["uri"]}") |
There was a problem hiding this comment.
another lint issue with f-string!
There was a problem hiding this comment.
🤦 sorry - tomorrow will setup and double check from personal laptop (should have done this to start with)
* Support HA and kerberos * reformat * Remove kerberos auth * Capture all exceptions * Make more pythonic * Add uts * Update UT to use assert_called_once_with * Fix for linting Co-authored-by: Kevin Liu <kevinjqliu@users.noreply.github.com> * Fix f string * fix formatting --------- Co-authored-by: Kevin Liu <kevinjqliu@users.noreply.github.com>
* Support HA and kerberos * reformat * Remove kerberos auth * Capture all exceptions * Make more pythonic * Add uts * Update UT to use assert_called_once_with * Fix for linting Co-authored-by: Kevin Liu <kevinjqliu@users.noreply.github.com> * Fix f string * fix formatting --------- Co-authored-by: Kevin Liu <kevinjqliu@users.noreply.github.com>
3 participants
Changes Proposed in this PR
Support a HA HMS
URI such as
uri: thrift://hms-1:9083,thrift://hms-2:9083currently is not supported. This change supports HA HMS were each entry will be tried until a successful connectionCloses #135